🛡️ Sentinel: [CRITICAL] Fix TOCTOU and DoS via file stat checks - #81
🛡️ Sentinel: [CRITICAL] Fix TOCTOU and DoS via file stat checks#81ivangegovdve-sudo wants to merge 1 commit into
Conversation
Co-authored-by: ivangegovdve-sudo <225339531+ivangegovdve-sudo@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates several file-backed adapters to mitigate TOCTOU vulnerabilities and DoS risks by replacing file size stat checks with bounded reads and ensuring paths are regular files. Feedback suggests enhancing robustness in the checkpoint store by handling potential file and JSON parsing errors. Additionally, it is recommended to replace the recurring magic number for the file size limit with a shared constant to improve maintainability across the adapters.
| with open(path, encoding="utf-8") as f: | ||
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: | ||
| raise ValueError(f"Checkpoint file {path} exceeds 10MB size limit") | ||
|
|
||
| parsed = json.loads(content) | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
This implementation can be improved in two ways:
- Robustness: The code doesn't handle potential
OSErrorfrom file operations orjson.JSONDecodeErrorfrom parsing. This could lead to unhandled exceptions, whereas other adapters in the codebase handle these gracefully. For consistency and robustness, this one should too. - Maintainability: The file size limit
10 * 1024 * 1024is a magic number. Defining it as a constant improves readability and makes it easier to change, especially since it's used across multiple files.
Here's a suggestion that addresses both points. Ideally, the MAX_FILE_SIZE_BYTES constant would be defined at the module level to be shared across all adapters.
MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 # 10MB
try:
with open(path, encoding="utf-8") as f:
content = f.read(MAX_FILE_SIZE_BYTES + 1)
if len(content) > MAX_FILE_SIZE_BYTES:
raise ValueError(f"Checkpoint file {path} exceeds 10MB size limit")
if not content.strip():
return {}
parsed = json.loads(content)
return parsed if isinstance(parsed, dict) else {}
except (OSError, json.JSONDecodeError, ValueError):
return {}| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is a magic number. To improve readability and maintainability, consider defining it as a shared constant (e.g., _MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024) at the module level or in a common constants file and using it here and in the other adapters.
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is a magic number. To improve readability and maintainability, consider defining it as a shared constant (e.g., _MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024) at the module level or in a common constants file and using it here and in the other adapters.
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is a magic number. To improve readability and maintainability, consider defining it as a shared constant (e.g., _MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024) at the module level or in a common constants file and using it here and in the other adapters.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Used
path.stat().st_sizeto check file size before reading the entire file, which exposes a Time-of-Check to Time-of-Use (TOCTOU) vulnerability. Additionally, using.exists()instead of.is_file()allows device files (like/dev/zero) to report a size of 0 and bypass the check, leading to an unbounded memory consumption DoS attack whenread_text()is called.🎯 Impact: Could allow an attacker or misconfiguration to crash the application via out-of-memory errors (DoS) by pointing to a special device file or exploiting the race condition.
🔧 Fix: Replaced
.exists()with.is_file()and used a secure bounded readf.read(limit + 1)and explicitopen()context manager to enforce the size limit directly at read time, entirely eliminating both the TOCTOU race condition and the/dev/zerodevice file bypass.✅ Verification: Ran
uv run pytestand verified all tests pass, confirmed bounded reads correctly raiseValueErrorfor large files via inspection, and recorded learnings in.jules/sentinel.md.PR created automatically by Jules for task 5613353413879271950 started by @ivangegovdve-sudo